Convert all scripts/ to typescript - #233

preview.ts is not part of this PR, so the stale build-release.mjs comment there is pre-existing and out of scope. Not a finding for this review.

I've thoroughly reviewed the PR. Let me summarize my findings. There are no actionable, blocking issues introduced by this change:

• All 129 script tests pass under Node 22 native type stripping; types:scripts passes.
• Only erasable syntax is used (enforced by erasableSyntaxOnly in scripts/tsconfig.json).
• No orphaned .test.js/.test.mjs files remain that the narrowed test glob would skip.
• No runtime-breaking .js/.mjs import specifiers of renamed files.
• The run-dev-server.js → scripts/run-dev-server.ts move correctly recomputes ROOT.
• The kill-process-tree / run-dev-server pid guard relocation is semantically equivalent.
• Type dedup (moving wrangler-config shapes into manifest-lib.ts) is a genuine improvement.
• The contribution-policy.ts dynamic import via actions/github-script@v9 (node24) is correctly reasoned and documented.
• Docs (AGENTS.md, skills, READMEs) accurately updated.

One non-blocking observation worth raising to the author (not an inline finding, since it's a repo-wide concern rather than a specific defect on a changed line):

The approach depends on Node ≥22.18.0 for native type stripping, but the repo adds no engines field to package.json. CI is pinned to 24.19.0 so CI is safe, but a contributor on older Node will hit opaque failures running pnpm run-local, pnpm dev-server, or pnpm test. Adding "engines": { "node": ">=22.18.0" } would surface a clear error instead. This is optional and doesn't block the change.

LGTM!

github run

Preview: maximo-sc-258e62be

https://maximo-sc-258e62be-os-public-router.gadgets-staging.workers.dev

Dashboard · deleted when this PR closes

Pre-existing and unmodified — out of scope.

My review is complete. Let me summarize findings. I verified the substantive claims:

• types:scripts passes (tsc clean).
• All 129 script tests pass under Node native type stripping; the golden-manifest contract test passes; the gatekeeper-configurator test passes.
• hash-lib.ts: the MODULE_TYPE_BY_EXTENSION lookup can now yield undefined, correctly guarded by the existing if (!type) throw (fail-closed semantics preserved).
• run-dev-server.ts move into scripts/: ROOT correctly recomputed as join(SCRIPTS_DIR, ".."); all ROOT/SCRIPTS_DIR path references consistent.
• kill-process-tree.ts: ESRCH handling preserved via cast; the watcher-kill pid guard relocation (filter → map returning null) is harmless under Promise.all.
• dev-server-config.ts, contribution-policy.ts, and the release scripts: only type annotations/doc comments added, no logic drift.
• Type consolidation (wrangler-config shapes moved into manifest-lib.ts, shared by preview generator) is a genuine dedup.
• No orphaned .test.js/.test.mjs under the narrowed test glob; all .mjs/.js script references updated across configs, docs, workflow, .gitignore.
• contribution-policy.ts runs via actions/github-script@v9 (node24), which supports type stripping.
• The only remaining stale .mjs reference (preview.ts:202) is pre-existing and in a file this PR does not modify — out of scope.

No actionable, blocking issues introduced by this change. This aligns with the prior review's conclusion; I found nothing additional to add as inline findings.

LGTM!

github run

The PR file list marked dev-server-config.ts as MODIFIED, but it was already .ts in the base branch (maximo/pr-previews). The diff shows only type annotations added. This matches the prior review's conclusion.

I've independently verified all the substantive aspects of this PR:

Verified:

• hash-lib.ts: faithful port; the MODULE_TYPE_BY_EXTENSION lookup can yield undefined (widened type via cast), correctly guarded by the existing if (!type) throw — fail-closed semantics preserved.
• kill-process-tree.ts: faithful; the pid > 0 guard relocation into killProcessTree is semantically equivalent; ESRCH handling preserved.
• run-dev-server.ts (moved root → scripts/): ROOT correctly recomputed as join(SCRIPTS_DIR, ".."); the .map(w => w.pid ? kill : null) into Promise.all is harmless.
• contribution-policy.ts: exports enforceContributionPolicy, uses only erasable syntax, runs under actions/github-script@v9 (node24, supports type stripping).
• staging-config.ts (-65 lines): genuine dedup — duplicated wrangler-config types now imported from manifest-lib.ts, which is a compatible superset.
• tsconfig.json enforces erasableSyntaxOnly + verbatimModuleSyntax; noEmit at root.
• package.json test glob narrowed to *.test.ts; no orphaned .test.js/.test.mjs remain.
• No stale runtime .mjs/.js import specifiers of renamed files (oxlint-plugin.mjs intentionally stays JS and is still referenced correctly).
• vite.config.ts dropped root-level '*.js', '*.mjs' lint glob — no root-level JS files remain, so no coverage lost.
• Docs (AGENTS.md, skills, READMEs, .gitignore, workflow) references updated consistently.

The one non-blocking observation (no engines field pinning Node ≥22.18.0) was already raised by the prior reviews; CI is pinned to 24.19.0, so CI is safe. This is a repo-wide concern rather than a defect on a changed line, so it's not an inline finding.

I found no actionable, blocking issues introduced by this change.

LGTM!

github run

Base: .filter(... && watcher.pid).map(watcher => killProcessTree(watcher.pid)...).

New (run-dev-server.ts:161-162): .filter(watcher => watcher.exitCode === null && watcher.signalCode === null).map(watcher => watcher.pid ? killProcessTree(watcher.pid)... : null).

The difference: base filtered out !watcher.pid before mapping; new keeps them and maps to null. Under Promise.all, a null entry is a resolved non-promise value — harmless. The reason for the change: TypeScript can't narrow watcher.pid from a number | undefined to number through a .filter() predicate (filter doesn't narrow the element type), so the ternary-in-map is needed to satisfy killProcessTree(pid: number). Semantically equivalent. This is a valid TS-motivated refactor.

I've completed a thorough independent review. Let me summarize my findings.

Review Summary

I independently verified this PR (converting all scripts/ to TypeScript, run natively via Node's type stripping):

Verified correct:

• Installed deps and ran the full suite: all 129 script tests pass under Node 22.23.2 native type stripping (matching the ≥22.18.0 requirement).
• pnpm run types:scripts passes clean (tsc under scripts/tsconfig.json with erasableSyntaxOnly + verbatimModuleSyntax).
• hash-lib.ts / kill-process-tree.ts: faithful ports; the MODULE_TYPE_BY_EXTENSION lookup widened to possibly-undefined is correctly guarded by the existing if (!type) throw (fail-closed preserved); ESRCH handling and the positive-integer pid guard preserved.
• run-dev-server.ts (moved repo-root → scripts/): ROOT correctly recomputed as join(SCRIPTS_DIR, ".."); all path references consistent.
• The stopDevWatchersDeep guard relocation (.filter(... && pid) → .map(w => w.pid ? kill : null)) is a TS-necessitated change (filter predicates don't narrow element types) and is semantically equivalent under Promise.all.
• staging-config.ts (−65 lines): genuine dedup — the wrangler-config shapes now import from manifest-lib.ts, whose WranglerConfig is a compatible superset (StagingConfig extends WranglerConfig, confirmed by the clean type-check).
• Test glob narrowed to scripts/**/*.test.ts; all 11 test files converted, no orphaned .test.js/.test.mjs remain.
• No stale runtime .mjs/.js import specifiers of renamed files; oxlint-plugin.mjs intentionally stays .mjs and is still referenced correctly. Remaining .mjs mentions (build-app.mjs, build-format-blueprints.mjs, the extension-map entry) are all legitimate/out-of-scope.
• Docs (AGENTS.md, skills, READMEs, .gitignore, workflow, vite.config.ts lint glob) updated consistently.

Non-blocking (out of scope / already raised):

• scripts/vitest-task-vite-config.ts:8 comment says "the .mjs/.js around it" but only oxlint-plugin.mjs remains — mildly stale, but that file is not modified by this PR.
• No engines field pinning Node ≥22.18.0 (CI is pinned to 24.19.0, so CI is safe) — already raised by prior reviews; a repo-wide concern, not a defect on a changed line.

I found no actionable, blocking issues introduced by this change.

LGTM!

github run

github run

Failed to check permissions for user Maximo-Guk: HttpError: No server is currently available to service your request. Sorry about that. Please try resubmitting your request and contact us if the problem persists.

github run

@Maximo-Guk Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

@Maximo-Guk Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

github run

Preview: maximo-scripts-typescript

https://maximo-scripts-typescript-router.cloudflare-os-previews.workers.dev

Dashboard · deleted when this PR closes